Database and Dataset Imports #47
Conversation
|
@orangewolf I will take a look after you rebase |
There was a problem hiding this comment.
Pull Request Overview
Adds directory-based import support and a YAML loader service for Superset database and dashboard imports.
- Introduces
Superset::Services::Loaderto unzip export archives and load YAML configs. - Updates
Superset::Database::ImportandSuperset::Dashboard::Importto accept both zip files and directories with unified validation and payload logic. - Expands specs to cover directory vs. zip import scenarios and various error conditions.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/superset/database/import_spec.rb | New tests for database import covering directory and zip sources, valid responses, and error cases |
| spec/superset/dashboard/import_spec.rb | Enhanced dashboard import specs to support directory sources and mirror database import behavior |
| lib/superset/services/loader.rb | New Loader service to extract ZIP archives and load YAML files into structured configs |
| lib/superset/database/import.rb | Extended database import class to handle directories, validate params, and build request payload |
| lib/superset/dashboard/import.rb | Refactored dashboard import to accept directories, use loader service, and unify validation |
Comments suppressed due to low confidence (2)
lib/superset/services/loader.rb:57
- [nitpick] The parameter name 'pattern_sufix' appears to be a typo; consider renaming it to 'pattern_suffix' for clarity.
def load_yamls_for(object_path, pattern_sufix: "**/*.yaml")
lib/superset/dashboard/import.rb:113
Superset::Services::DashboardLoaderis not defined in the codebase; ensure the correct loader class is referenced or implemented.
Superset::Services::DashboardLoader.new(dashboard_export_zip: source).perform
|
|
||
| def new_zip_file | ||
| database_name = database_config[:databases].first[:content][:database_name] | ||
| File.join(source, "database_import.zip") |
There was a problem hiding this comment.
[nitpick] The local variable database_name is assigned but never used; remove it or use it in the ZIP path if that was the intention.
| File.join(source, "database_import.zip") | |
| zip_filename = database_name ? "#{database_name}_import.zip" : "database_import.zip" | |
| File.join(source, zip_filename) |
Copilot uses AI. Check for mistakes.
| raise ArgumentError, "source is not a zip file or directory" unless zip? || directory? | ||
| raise ArgumentError, "overwrite must be a boolean" unless [true, false].include?(overwrite) | ||
|
|
||
| return unless database_config_not_found_in_superset.present? |
There was a problem hiding this comment.
Using present? requires ActiveSupport; replace with a Ruby core method like !database_config_not_found_in_superset.empty? to avoid a NoMethodError.
| return unless database_config_not_found_in_superset.present? | |
| return if database_config_not_found_in_superset.empty? |
Copilot uses AI. Check for mistakes.
| end | ||
|
|
||
| def new_zip_file | ||
| new_database_name = dashboard_config[:databases].first[:content][:database_name] |
There was a problem hiding this comment.
[nitpick] The variable new_database_name is set but never used; remove it or incorporate it into the generated file name if intended.
| new_database_name = dashboard_config[:databases].first[:content][:database_name] |
Copilot uses AI. Check for mistakes.
Sounds great. I'll do my best to get it this week. |
builds on the work done in #46 - review / merge it first